Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JENKINS-71578] allow making sidepanel sticky #8269

Merged
merged 9 commits into from
Dec 19, 2023

Conversation

mawinter69
Copy link
Contributor

@mawinter69 mawinter69 commented Jul 14, 2023

Having a sticky sidepanel allows pages with only few links but longer content easier navigation.

Apply stickiness to PluginManager.

After:
image

See JENKINS-71578.

Testing done

Manually visit PluginManager
Sidepanel sticks when scrolling down so all links stay accessible all the time.
Configure a job -> sidepanel is still sticky

Proposed changelog entries

  • Allow to make side panel sticky

Proposed upgrade guidelines

N/A

Submitter checklist

Desired reviewers

@mention

Before the changes are marked as ready-for-merge:

Maintainer checklist

Having a sticky sidebar not only for app bars allows pages with only few
links but longer content easier navigation
Copy link
Member

@daniel-beck daniel-beck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs to be accompanied by (plugin) developer guidance when to use this to prevent a mess of arbitrary choices.

Should not be used on extensible pages like for User, otherwise scrolling makes later entries inaccessible. (It's fairly unlikely to matter for User, but it's an extensible sidepanel nonetheless.)

@mawinter69
Copy link
Contributor Author

The non scrolling is already a problem now. Just go to configure job and make the browser window very small and the lower links are gone.
So maybe we should add some css to allow it to scroll if required

@daniel-beck
Copy link
Member

daniel-beck commented Jul 14, 2023

already a problem now

While small windows will hide sidepanel entries for config forms, it's the same 5 or so entries every time, and they're only for navigation inside the same page (something you can also accomplish by scrolling), not to go somewhere else. User and everything Actionable has an unbounded number of sidepanel entries, and those links go elsewhere with usually no alternative.

So maybe we should add some css to allow it to scroll if required

Would be an improvement in any case I think 👍

Which leaves when to use which. If scrolling sidepanels will be able to handle arbitrary numbers of entries nicely, and are better, why should this be optional, and opt-in?

when to use it and when not
@mawinter69
Copy link
Contributor Author

tested this on a job. When adding a overflow: auto and a scrollbar is shown, the widget fills up to the scrollbar and then when hovering the navigation arrows are missing. Can be overcome by adding padding to the sidepanel but this will increase the gap between side and main panel. And scrollbar-gutter is not supported on Safari 😞

@janfaracik
Copy link
Contributor

Would it be possible to move <div id="side-panel" class="app-page-body__sidebar"> from layout.jelly into side-panel.jelly and that way we could do away with the JS?

@mawinter69
Copy link
Contributor Author

mawinter69 commented Aug 17, 2023

Would it be possible to move <div id="side-panel" class="app-page-body__sidebar"> from layout.jelly into side-panel.jelly and that way we could do away with the JS?

done
A minor difference would be if someone creates a two column layout but is not including a sidepanel, then the div is not generated which might be expected when enabling YUI debug mode.

Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@timja timja added the developer Changes which impact plugin developers label Aug 19, 2023
@timja timja requested a review from daniel-beck August 19, 2023 17:06
@github-actions github-actions bot added the unresolved-merge-conflict There is a merge conflict with the target branch. label Aug 21, 2023
@github-actions
Copy link
Contributor

Please take a moment and address the merge conflicts of your pull request. Thanks!

@github-actions github-actions bot removed the unresolved-merge-conflict There is a merge conflict with the target branch. label Aug 23, 2023
@timja timja added the web-ui The PR includes WebUI changes which may need special expertise label Dec 17, 2023
@timja
Copy link
Member

timja commented Dec 17, 2023

/label ready-for-merge


This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

@comment-ops-bot comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Dec 17, 2023
@NotMyFault NotMyFault merged commit a822935 into jenkinsci:master Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
developer Changes which impact plugin developers ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback web-ui The PR includes WebUI changes which may need special expertise
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants